Treat XDG_CONFIG_HOME and $HOME/.config the same way #2084

rhatdan · 2024-09-04T10:23:08Z

Currently we handle XDG_CONFIG_HOME and the fallback $HOME/.config differently. We create the later if it does not exist and verify that it is owned by the current user. This patch makes XDG_CONFIG_HOME follow the same pattern.

openshift-ci · 2024-09-04T10:23:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2024-09-04T10:28:11Z

Can we first get an agreement what this should do? I still like to push hard that this function should not try to create and state the directory at all.

rhatdan · 2024-09-04T12:27:42Z

I have no problem with that other then it is potentially a breaking change.

mtrmac

This is presuming a consensus on something like containers/podman#23818 (comment) ; I’m not sure we have that yet.

mtrmac · 2024-09-04T16:26:09Z

pkg/homedir/homedir_unix.go

+// ConfigHome returns $HOME/.config and nil error if XDG_CONFIG_HOME is not set.
+// Verifies ownership of config directory matches the current process or
+// returns error.
+// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html


(This link is now broken. Maybe https://specifications.freedesktop.org/basedir-spec/latest/index.html#variables ?)

mtrmac · 2024-09-04T16:26:47Z

pkg/homedir/homedir_unix.go

+// Verifies ownership of config directory matches the current process or
+// returns error.
+// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
+func GetConfigHome() (string, error) {


This does not compile; was this intended to be ConfigHome?

mtrmac · 2024-09-04T16:27:20Z

pkg/homedir/homedir_unix.go

+// returns error.
+// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
+func GetConfigHome() (string, error) {
+	rootlessConfigHomeDirOnce.Do(func() {


If this is a new function with a new semantics, the …{DirOnce,Dir,DirError} variables must not be shared with the other function.

(sync.OnceValues now exists. Aesthetically I don’t think it’s all that great, but it does have one advantage of allowing the nested function to use an ordinary return value, err control flow.)

pkg/homedir/homedir_unix.go

mtrmac · 2024-09-04T16:58:07Z

pkg/homedir/homedir_unix.go

+	return rootlessConfigHomeDir, rootlessConfigHomeDirError
+}
+
+// GetConfigHome returns XDG_CONFIG_HOME. (Deprecated)


If we go with the plan of gradual migration per containers/podman#23818 (comment) , please

point at the alternative

explain why it this one is deprecated / what needs to change in callers

consider using the standard Deprecated: syntax that linters understand. OTOH that would force us to migrate immediately, I’m not 100% sure we want to sign up for doing that

Currently we handle XDG_CONFIG_HOME and the fallback $HOME/.config differently. We create the later if it does not exist and verify that it is owned by the current user. This patch makes XDG_CONFIG_HOME follow the same pattern. Also Deprecate all GetDATA() functions and just use DATA(). Signed-off-by: Daniel J Walsh <[email protected]>

mtrmac · 2024-09-05T19:13:40Z

pkg/homedir/homedir_unix.go

 func GetConfigHome() (string, error) {
+	rootlessConfigHomeDir, rootlessConfigHomeDirError = ConfigHome()
+	if rootlessConfigHomeDirError == nil {
+		_ = os.MkdirAll(rootlessConfigHomeDir, 0o700)


If this function is deprecated and we intend to migrate all users, I don’t see that changing the existing one to not report failures is useful: that forces us to immediately review/migrate all users, negating the benefit of the deprecation and introduction of a new name for the new version.

Also, right now, if the directory is missing, ConfigHome returns ("", ENOENT), so AFAICS this condition does nothing.

Shouldn’t all these functions, with their rare corner cases we don’t encounter on our workstations, have pretty good test coverage? I still mourn for tests lost in #1740 .

mtrmac · 2024-09-05T19:14:15Z

pkg/homedir/homedir_unix.go

+	return rootlessConfigHomeDir, rootlessConfigHomeDirError
+}
+
+// ConfigHome returns XDG_CONFIG_HOME. (Deprecated)


Presumably this one is not deprecated.

mtrmac · 2024-09-05T19:16:36Z

pkg/homedir/homedir.go

@@ -6,11 +6,16 @@ import (
 	"path/filepath"
 )

-// GetDataHome returns XDG_DATA_HOME.
-// GetDataHome returns $HOME/.local/share and nil error if XDG_DATA_HOME is not set.
+// DataHome (deprecated)


For all these name changes…

I don’t really see enough benefit but also I don’t really care

I do think the code should either:

Introduce a new name for symmetry, and intend to keep the old names forever. Then it’s not really deprecated.

Decide to deprecate the old name; in that case this should use the standard Deprecated: syntax so that tool make us migrate immediately.

… all these name changes apart from the real semantic change WRT ConfigHome.

I rather not do the unrelated name changes, it just adds churn for no reason. If I look at a code and see the two different functions used I will always have to follow them to figure out if they are actual equal.

Luap99 · 2024-09-06T09:20:25Z

pkg/homedir/homedir_unix.go

-			}
+			cfgHomeDir = filepath.Join(resolvedHome, ".config")
+		}
+		st, err := os.Stat(cfgHomeDir)


I am still not convinced that the stat adds value here.

In fact if we keep the the stat then I see zero reason to change anything here because then most callers still have to ignore ENOENT errors which adds a lot of code and doesn't help the issue containers/podman#23818.

Neither HomeDir() or GetCacheHome() validate permissions either.

I agree that propagating ENOENT to callers is probably undesirable.

To be specific, we need some plan for the podman machine code that writes to the config directory. If the config directory doesn’t exist, how does it get created? The current version of ConfigHome seems not to allow that to happen at all.

Neither HomeDir() or GetCacheHome() validate permissions either.

We’ve had past painful experience with users who have XDG_*DIR set in the environment using su (without --login) and clobbering other users’ accounts (and if root is overwriting other users’ data, that other user might not be able to recover without help).

I think if we ignore or special-case ENOENT, the permission checks have no downsides, and should be preserved.

openshift-ci bot added the approved label Sep 4, 2024

rhatdan mentioned this pull request Sep 4, 2024

Do not fail on $HOME not containing .config directory containers/common#2147

Open

rhatdan force-pushed the config branch from 4cf4d20 to f74b71b Compare September 4, 2024 12:48

mtrmac reviewed Sep 4, 2024

View reviewed changes

rhatdan force-pushed the config branch from f74b71b to cd53592 Compare September 5, 2024 18:59

rhatdan force-pushed the config branch from cd53592 to 0285387 Compare September 5, 2024 19:08

mtrmac reviewed Sep 5, 2024

View reviewed changes

Luap99 reviewed Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat XDG_CONFIG_HOME and $HOME/.config the same way #2084

Treat XDG_CONFIG_HOME and $HOME/.config the same way #2084

rhatdan commented Sep 4, 2024

openshift-ci bot commented Sep 4, 2024

Luap99 commented Sep 4, 2024

rhatdan commented Sep 4, 2024

mtrmac left a comment

mtrmac Sep 4, 2024

mtrmac Sep 4, 2024

mtrmac Sep 4, 2024

mtrmac Sep 4, 2024 •

edited

Loading

mtrmac Sep 5, 2024

mtrmac Sep 6, 2024

mtrmac Sep 5, 2024

mtrmac Sep 5, 2024

mtrmac Sep 5, 2024

Luap99 Sep 6, 2024

Luap99 Sep 6, 2024

mtrmac Sep 6, 2024

Treat XDG_CONFIG_HOME and $HOME/.config the same way #2084

Are you sure you want to change the base?

Treat XDG_CONFIG_HOME and $HOME/.config the same way #2084

Conversation

rhatdan commented Sep 4, 2024

openshift-ci bot commented Sep 4, 2024

Luap99 commented Sep 4, 2024

rhatdan commented Sep 4, 2024

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Sep 4, 2024 •

edited

Loading